Initial stats cleanup#1009
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
| RmmResourceAdaptor mr, | ||
| std::optional<PinnedMemoryResource> pinned_mr = PinnedMemoryResource::Disabled | ||
| ); | ||
| static std::shared_ptr<Statistics> create(bool enabled = true); |
There was a problem hiding this comment.
question: What does having a create static method buy us over a constructor?
There was a problem hiding this comment.
factory methods allows us to safely use std::enable_shared_from_this, where we can pass a shared_ptr to memory recorder rather than a ptr
nirandaperera
force-pushed
the
stats-refactor
branch
from
2889f62 to
8236c70
Compare
Signed-off-by: niranda perera <niranda.perera@gmail.com> remove stats from allgather Signed-off-by: niranda perera <niranda.perera@gmail.com> stats provider concept Signed-off-by: niranda perera <niranda.perera@gmail.com> WIP Signed-off-by: niranda perera <niranda.perera@gmail.com>
nirandaperera
force-pushed
the
stats-refactor
branch
from
8236c70 to
0c6cc9c
Compare
Signed-off-by: niranda perera <niranda.perera@gmail.com>
Signed-off-by: niranda perera <niranda.perera@gmail.com>
nirandaperera
added
improvement
|
@wence- this is ready for review now |
pentschev
left a comment
pentschev
left a comment
There was a problem hiding this comment.
At a high-level this looks ok. Left some minor suggestions.
| * @return Shared pointer the Statistics instance. | ||
| */ | ||
| std::shared_ptr<Statistics> statistics(); | ||
| std::shared_ptr<Statistics> const& statistics() const noexcept; |
There was a problem hiding this comment.
This is OK as you're going for a hot-path optimization, but the lifetime contract should be explicit. Otherwise returning shared_ptr by value is the safer and more idiomatic API.
| * @param statistics The statistics instance to use (disabled by default). The caller | ||
| * is responsible for creating and owning this object. |
There was a problem hiding this comment.
"and owning this object" doesn't make sense here, std::shared_ptr<Statistics> is passed by value.
| * by-value `std::shared_ptr` return would incur on hot paths. | ||
| * | ||
| * Each provider asserts this concept via `static_assert` in its own header. | ||
| * Current providers: `BufferResource`, `ProgressThread`, `streaming::Context`. |
There was a problem hiding this comment.
| * Current providers: `BufferResource`, `ProgressThread`, `streaming::Context`. |
There was a problem hiding this comment.
Probably best not to document providers here as this will eventually fall out-of-sync.
3 participants
Statisticswas constructible directly (make_shared<Statistics>(false)was not equal toStatistics::disabled()),MemoryRecorderheld a rawStatistics*(lifetime hazard noted by an existing TODO), providerstatistics()accessors returnedshared_ptrby value (atomic refcount bumps on hot paths), and secondary objects likeAllGatherredundantly carried their ownstatistics_member already reachable through the buffer resource / context.In this PR, following changes were made
Statisticsconstructible only through factory methods (create(),from_options(),disabled()); the public ctor is removed anddisabled()is now the canonical singleton for the disabled case.Statisticsfromstd::enable_shared_from_thissoMemoryRecorderowns ashared_ptr<Statistics>instead of a raw pointer; recorder no-ops when stats are disabled.StatisticsProviderconcept and have providers (BufferResource,ProgressThread,streaming::Context) returnstd::shared_ptr<Statistics> const&fromstatistics(); each providerstatic_asserts the concept.statisticsparameter andstatistics_member fromcoll::AllGather(and its Python binding); secondary classes now derive theirStatisticsfrom the provider they already receive.Related to #1008
Depends on #1003